lint/ctypes: ext. abi fn-ptr in internal abi fn #108611

davidtwco · 2023-03-01T15:59:08Z

Fixes #94223.

In the improper ctypes lint, instead of skipping functions with internal ABIs, check that the signature doesn't contain any fn-ptr types with external ABIs that aren't FFI-safe.
When computing the ABI for fn-ptr types, remove an unwrap that assumed FFI-safe types in foreign fn-ptr types.
- I'm not certain that this is the correct approach.

rustbot · 2023-03-01T15:59:20Z

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

compiler/rustc_lint/src/types.rs

Noratrieb · 2023-03-01T16:56:11Z

This seems to be the wording that is already used, but I really don't like the internal vs external| wording here, it makes no sense. An extern "Rust" fnis not internal, it just has a Rust ABI. Maybec_likevsrust_like` would be a better name.

davidtwco · 2023-03-02T12:03:19Z

This seems to be the wording that is already used, but I really don't like the internal vs external| wording here, it makes no sense. An extern "Rust" fnis not internal, it just has a Rust ABI. Maybec_likevsrust_like` would be a better name.

I agree, but I'll leave this for a follow-up.

WaffleLapkin

I'm a bit confused now.

Surely this should fail compilation:

#![deny(improper_ctypes_definitions, improper_ctypes)]
struct __(extern "C" fn([u8]));

And yet, it currently passes it ([play]). And, if I understood correctly, under this PR it would pass too...

Maybe instead of trying to deeply check types in extern items, we should actually just check all types, find extern ... fn(...) types and check their arguments?

davidtwco · 2023-03-06T14:11:08Z

Maybe instead of trying to deeply check types in extern items, we should actually just check all types, find extern ... fn(...) types and check their arguments?

I've extended support to all types that could contain the extern fn-ptr types (which I think is the only thing that we need to check in otherwise okay types, i.e. fn not extern fn).

WaffleLapkin · 2023-03-06T17:05:52Z

I've extended support to all types that could contain the extern fn-ptr types

I don't think that's necessarily enough.

For example this doesn't get linted, still:

#![deny(improper_ctypes_definitions, improper_ctypes)]
fn main() {
    let _: extern "C" fn([u8]) = todo!();
}

I would prefer using something like LateLintPass::check_ty, so that we are sure we lint every type that is written in the source... but check_ty gets only a hir::Ty so I'm not sure if it could work (I don't see an easy way to get rustc_middle::ty::Ty from it)

davidtwco · 2023-03-06T17:11:11Z

I would prefer using something like LateLintPass::check_ty, so that we are sure we lint every type that is written in the source... but check_ty gets only a hir::Ty so I'm not sure if it could work (I don't see an easy way to get rustc_middle::ty::Ty from it)

That's exactly why I haven't used check_ty, also because I didn't want to check anything in foreign items, as the ImproperCTypesDeclarations lint visitor handles those.

~~I think all I can do is implement check_expr? Not sure where I can get a hir::Ty for that though..~~ I could implement check_local to handle that case, but not sure if there others this would miss?

WaffleLapkin · 2023-03-07T10:00:22Z

idk, I don't feel confident reviewing this PR, so I'll reroll
r? compiler

apiraino · 2023-04-20T10:21:12Z

I assume Wesley has not enough bandwidth right now, so I'll reroll

r? compiler

jackh726

Just one thought, but r=me if you don't want to try to change things.

jackh726 · 2023-04-23T17:40:33Z

compiler/rustc_lint/src/types.rs

        let sig = self.cx.tcx.fn_sig(def_id).subst_identity();
        let sig = self.cx.tcx.erase_late_bound_regions(sig);

+        let is_internal_abi = self.is_internal_abi(abi);
+        let check_ty = |this: &mut ImproperCTypesVisitor<'a, 'tcx>,


So, this all seems like it should just be a part of a visitor with a flag for foreign abi or not.

davidtwco · 2023-04-24T10:50:06Z

@bors r=jackh726

I think that suggestion is best left for a follow-up.

bors · 2023-04-24T10:50:08Z

📌 Commit 3e7ca3e has been approved by jackh726

It is now in the queue for this repository.

bors · 2023-04-25T02:01:10Z

⌛ Testing commit 3e7ca3e with merge f13b0551bf4f1aa76569c80852751b51c811f5fc...

bors · 2023-06-29T08:18:45Z

📌 Commit f14e5778ca8dccef35f243cb8b7869fe239afb69 has been approved by jackh726

It is now in the queue for this repository.

bors · 2023-06-29T10:02:28Z

⌛ Testing commit f14e5778ca8dccef35f243cb8b7869fe239afb69 with merge be32e03c64ad0401ea676f7831ad574bc7740354...

Instead of skipping functions with internal ABIs, check that the signature doesn't contain any fn-ptr types with external ABIs that aren't FFI-safe. Signed-off-by: David Wood <[email protected]>

Remove an `unwrap` that assumed FFI-safe types in foreign fn-ptr types. Signed-off-by: David Wood <[email protected]>

Extend previous commit's support for checking for external fn-ptrs in internal fn types to report errors for multiple found fn-ptrs. Signed-off-by: David Wood <[email protected]>

Extend previous checks for external ABI fn-ptrs to use in internal statics, constants, type aliases and algebraic data types. Signed-off-by: David Wood <[email protected]>

This was causing compilation failures in the performance benchmarking as diesel hit recursion limits. Signed-off-by: David Wood <[email protected]>

Signed-off-by: David Wood <[email protected]>

davidtwco · 2023-07-03T14:30:49Z

@bors r=jackh726

bors · 2023-07-03T14:30:51Z

📌 Commit 2227422 has been approved by jackh726

It is now in the queue for this repository.

bors · 2023-07-03T20:30:30Z

⌛ Testing commit 2227422 with merge 0ab38e9...

bors · 2023-07-03T22:57:50Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 0ab38e9 to master...

rust-timer · 2023-07-04T00:18:53Z

Finished benchmarking commit (0ab38e9): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	2.6%	[2.5%, 2.7%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.3%	[-1.7%, -0.9%]	2
All ❌✅ (primary)	2.2%	[2.2%, 2.2%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 670.457s -> 669.062s (-0.21%)

rustbot assigned WaffleLapkin Mar 1, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 1, 2023

WaffleLapkin reviewed Mar 1, 2023

View reviewed changes

compiler/rustc_lint/src/types.rs Outdated Show resolved Hide resolved

compiler/rustc_lint/src/types.rs Show resolved Hide resolved

davidtwco force-pushed the issue-94223-external-abi-fn-ptr-in-internal-abi-fn branch from eb578e5 to 5da1782 Compare March 2, 2023 12:44

WaffleLapkin reviewed Mar 2, 2023

View reviewed changes

davidtwco force-pushed the issue-94223-external-abi-fn-ptr-in-internal-abi-fn branch from 5da1782 to 3e7ca3e Compare March 6, 2023 14:08

rustbot assigned wesleywiser and unassigned WaffleLapkin Mar 7, 2023

rustbot assigned jackh726 and unassigned wesleywiser Apr 20, 2023

jackh726 approved these changes Apr 23, 2023

View reviewed changes

jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 24, 2023

This comment was marked as resolved.

Sign in to view

bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 25, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 29, 2023

This comment was marked as resolved.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 29, 2023

davidtwco added 6 commits July 3, 2023 13:40

lint/ctypes: ext. abi fn-ptr in internal abi fn

e3f90bc

Instead of skipping functions with internal ABIs, check that the signature doesn't contain any fn-ptr types with external ABIs that aren't FFI-safe. Signed-off-by: David Wood <[email protected]>

abi: avoid ice for non-ffi-safe fn ptrs

eddfce5

Remove an `unwrap` that assumed FFI-safe types in foreign fn-ptr types. Signed-off-by: David Wood <[email protected]>

lint/ctypes: multiple external fn-ptrs in ty

c75b080

Extend previous commit's support for checking for external fn-ptrs in internal fn types to report errors for multiple found fn-ptrs. Signed-off-by: David Wood <[email protected]>

lint/ctypes: check other types for ext. fn-ptr ty

9137fea

Extend previous checks for external ABI fn-ptrs to use in internal statics, constants, type aliases and algebraic data types. Signed-off-by: David Wood <[email protected]>

lint: stop normalizing types to avoid recur limits

f1e2879

This was causing compilation failures in the performance benchmarking as diesel hit recursion limits. Signed-off-by: David Wood <[email protected]>

lint: refactor to make logic a bit cleaner

2227422

Signed-off-by: David Wood <[email protected]>

davidtwco force-pushed the issue-94223-external-abi-fn-ptr-in-internal-abi-fn branch from f14e577 to 2227422 Compare July 3, 2023 13:29

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 3, 2023

bors merged commit 0ab38e9 into rust-lang:master Jul 3, 2023

rustbot added this to the 1.72.0 milestone Jul 3, 2023

bors mentioned this pull request Jul 3, 2023

Fix several occurences of the improper_ctypes lint in tests where that was not the intent #111972

Closed

davidtwco deleted the issue-94223-external-abi-fn-ptr-in-internal-abi-fn branch July 4, 2023 13:59

oxalica mentioned this pull request Jul 7, 2023

nightly/beta regression: fnptrs with types containing () is warned to be not FFI-safe, while it is before #113436

Closed

oxalica mentioned this pull request Jul 20, 2023

[ICE] when compiling abi_stable: Failed to normalize ..., maybe try to call try_normalize_erasing_regions instead #113900

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint/ctypes: ext. abi fn-ptr in internal abi fn #108611

lint/ctypes: ext. abi fn-ptr in internal abi fn #108611

davidtwco commented Mar 1, 2023 •

edited

Loading

rustbot commented Mar 1, 2023

Noratrieb commented Mar 1, 2023

davidtwco commented Mar 2, 2023

WaffleLapkin left a comment

davidtwco commented Mar 6, 2023

WaffleLapkin commented Mar 6, 2023

davidtwco commented Mar 6, 2023 •

edited

Loading

WaffleLapkin commented Mar 7, 2023

apiraino commented Apr 20, 2023

jackh726 left a comment

jackh726 Apr 23, 2023

davidtwco commented Apr 24, 2023

bors commented Apr 24, 2023

bors commented Apr 25, 2023

This comment was marked as resolved.

bors commented Jun 29, 2023

bors commented Jun 29, 2023

This comment was marked as resolved.

This comment was marked as resolved.

davidtwco commented Jul 3, 2023

bors commented Jul 3, 2023

bors commented Jul 3, 2023

bors commented Jul 3, 2023

rust-timer commented Jul 4, 2023

lint/ctypes: ext. abi fn-ptr in internal abi fn #108611

lint/ctypes: ext. abi fn-ptr in internal abi fn #108611

Conversation

davidtwco commented Mar 1, 2023 • edited Loading

rustbot commented Mar 1, 2023

Noratrieb commented Mar 1, 2023

davidtwco commented Mar 2, 2023

WaffleLapkin left a comment

Choose a reason for hiding this comment

davidtwco commented Mar 6, 2023

WaffleLapkin commented Mar 6, 2023

davidtwco commented Mar 6, 2023 • edited Loading

WaffleLapkin commented Mar 7, 2023

apiraino commented Apr 20, 2023

jackh726 left a comment

Choose a reason for hiding this comment

jackh726 Apr 23, 2023

Choose a reason for hiding this comment

davidtwco commented Apr 24, 2023

bors commented Apr 24, 2023

bors commented Apr 25, 2023

This comment was marked as resolved.

bors commented Jun 29, 2023

bors commented Jun 29, 2023

This comment was marked as resolved.

This comment was marked as resolved.

davidtwco commented Jul 3, 2023

bors commented Jul 3, 2023

bors commented Jul 3, 2023

bors commented Jul 3, 2023

rust-timer commented Jul 4, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

davidtwco commented Mar 1, 2023 •

edited

Loading

davidtwco commented Mar 6, 2023 •

edited

Loading